Skip to content

Add test suite for tekton-kueue CEL expressions #7561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 15, 2025

Conversation

gbenhaim
Copy link
Member

  • Add hack/test-tekton-kueue-config.py with parameterized unittest
  • Test all CEL mutation scenarios: multi-platform, priority assignment, queue config
  • Dynamically read image from kustomization and config from development env
  • Add GitHub action to run tests on kueue-related PRs

Validates tekton-kueue mutations against expected annotations, labels, and priority classes using actual container via podman.

Also, update the tekton-kueue config on dev to match staging.

Assisted-By: Cursor

Copy link
Contributor

Code Review by Gemini

## Review of Changes

The changes introduce a comprehensive test suite for Tekton-Kueue CEL expressions, a new GitHub Actions workflow to run these tests, and a minor update to the `tekton-kueue` configuration. The overall approach is excellent, providing robust validation for the mutation logic.

### `components/kueue/development/tekton-kueue/config.yaml`

**Improvement:**

The change to include `replace(..., "_", "-")` is a good improvement for normalizing platform names, ensuring consistency in annotation keys.

### `hack/test-tekton-kueue-config.py`

**Potential Issue/Improvement:**

The `get_tekton_kueue_image` function could be made more robust to handle different `kustomization.yaml` structures for image overrides. Currently, it strictly requires both `newName` and `newTag` to be present. If `newName` is omitted (meaning the original `name` should be used), the current logic would fail.

**Suggested Change:**

```diff
--- a/hack/test-tekton-kueue-config.py
+++ b/hack/test-tekton-kueue-config.py
@@ -29,16 +29,19 @@
         
         # Look for the tekton-kueue image in the images section
         images = kustomization.get('images', [])
         for image in images:
             if image.get('name') == 'konflux-ci/tekton-kueue':
-                new_name = image.get('newName', '')
-                new_tag = image.get('newTag', '')
-                if new_name and new_tag:
-                    return f"{new_name}:{new_tag}"
+                # Use 'name' as fallback for 'newName' if 'newName' is not specified
+                image_name = image.get('newName', image.get('name'))
+                image_tag = image.get('newTag')
+                
+                if image_name and image_tag:
+                    return f"{image_name}:{image_tag}"
+                else:
+                    raise ValueError(f"Incomplete tekton-kueue image definition found in kustomization. Expected 'newTag' and optionally 'newName' for '{image.get('name')}'.")
         
-        raise ValueError("tekton-kueue image not found in kustomization")
+        raise ValueError("tekton-kueue image entry not found in kustomization for 'konflux-ci/tekton-kueue'")
     
     except Exception as e:
         raise RuntimeError(f"Failed to read tekton-kueue image from {KUSTOMIZATION_FILE}: {e}")

.github/workflows/test-tekton-kueue-config.yaml

Minor Improvement (Optional):

The "Report success" step is largely cosmetic as the job status already indicates success. It can be removed to slightly reduce workflow verbosity, but it's harmless to keep.

No specific line changes are strictly necessary for this file, as its current state is functional and clear.


Overall Assessment:

The changes are well-structured and significantly improve the test coverage for Tekton-Kueue CEL expressions. The use of podman to run the actual container for mutations is a robust approach. The new GitHub Actions workflow ensures these tests are run automatically, catching issues early. The hack/test-tekton-kueue-config.py script is well-documented and uses good practices like unittest and tempfile.

Copy link

openshift-ci bot commented Aug 12, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gbenhaim, mshaposhnik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gbenhaim
Copy link
Member Author

/test appstudio-e2e-tests

Comment on lines 68 to 69
CONFIG_FILE = REPO_ROOT / "components/kueue/development/tekton-kueue/config.yaml"
KUSTOMIZATION_FILE = REPO_ROOT / "components/kueue/staging/base/tekton-kueue/kustomization.yaml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rely on pathlib to inject the correct path separator for the current OS

Suggested change
CONFIG_FILE = REPO_ROOT / "components/kueue/development/tekton-kueue/config.yaml"
KUSTOMIZATION_FILE = REPO_ROOT / "components/kueue/staging/base/tekton-kueue/kustomization.yaml"
CONFIG_FILE = REPO_ROOT / "components" / "kueue" / "development" / "tekton-kueue" / "config.yaml"
KUSTOMIZATION_FILE = REPO_ROOT / "components" / "kueue" / "staging" / "base" / "tekton-kueue" / "kustomization.yaml"```

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. it's already doing it. there is not need to provide it with a splitted path.

Comment on lines 446 to 491
print("Checking prerequisites...")

# Check config file
if CONFIG_FILE.exists():
print(f"✓ Config file found: {CONFIG_FILE}")
else:
print(f"✗ Config file not found: {CONFIG_FILE}")
sys.exit(1)

# Check kustomization file
if KUSTOMIZATION_FILE.exists():
print(f"✓ Kustomization file found: {KUSTOMIZATION_FILE}")
else:
print(f"✗ Kustomization file not found: {KUSTOMIZATION_FILE}")
sys.exit(1)

# Check tekton-kueue image
try:
image = get_tekton_kueue_image()
print(f"✓ Tekton-kueue image: {image}")
except Exception as e:
print(f"✗ Failed to get tekton-kueue image: {e}")
sys.exit(1)

# Check podman
try:
result = subprocess.run(["podman", "--version"], capture_output=True, check=True, text=True)
print(f"✓ Podman available: {result.stdout.strip()}")
except (subprocess.CalledProcessError, FileNotFoundError):
print("✗ Podman is not available")
sys.exit(1)

print("\n✅ All prerequisites met! Ready to run tests.")
print("Run: python hack/test-tekton-kueue-config.py")
print("\nNote: Tests will FAIL (not skip) if any prerequisites are missing.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: for readability, I'd suggest moving this into a dedicated function

def checkPrerequisites(): 
        # ... 
Suggested change
print("Checking prerequisites...")
# Check config file
if CONFIG_FILE.exists():
print(f"✓ Config file found: {CONFIG_FILE}")
else:
print(f"✗ Config file not found: {CONFIG_FILE}")
sys.exit(1)
# Check kustomization file
if KUSTOMIZATION_FILE.exists():
print(f"✓ Kustomization file found: {KUSTOMIZATION_FILE}")
else:
print(f"✗ Kustomization file not found: {KUSTOMIZATION_FILE}")
sys.exit(1)
# Check tekton-kueue image
try:
image = get_tekton_kueue_image()
print(f"✓ Tekton-kueue image: {image}")
except Exception as e:
print(f"✗ Failed to get tekton-kueue image: {e}")
sys.exit(1)
# Check podman
try:
result = subprocess.run(["podman", "--version"], capture_output=True, check=True, text=True)
print(f"✓ Podman available: {result.stdout.strip()}")
except (subprocess.CalledProcessError, FileNotFoundError):
print("✗ Podman is not available")
sys.exit(1)
print("\n✅ All prerequisites met! Ready to run tests.")
print("Run: python hack/test-tekton-kueue-config.py")
print("\nNote: Tests will FAIL (not skip) if any prerequisites are missing.")
checkPrerequisites()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 336 to 358
"""Set up test class - check prerequisites."""
# Check if config file exists
if not CONFIG_FILE.exists():
raise FileNotFoundError(f"Config file not found: {CONFIG_FILE}")

# Check if kustomization file exists
if not KUSTOMIZATION_FILE.exists():
raise FileNotFoundError(f"Kustomization file not found: {KUSTOMIZATION_FILE}")

# Get the tekton-kueue image from kustomization
try:
cls.tekton_kueue_image = get_tekton_kueue_image()
print(f"Using tekton-kueue image: {cls.tekton_kueue_image}")
except Exception as e:
raise RuntimeError(f"Failed to get tekton-kueue image: {e}")

# Check if podman is available
try:
subprocess.run(["podman", "--version"], capture_output=True, check=True)
except FileNotFoundError:
raise RuntimeError("podman command not found. Please install podman.")
except subprocess.CalledProcessError as e:
raise RuntimeError(f"podman is not working properly: {e}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to me this logic is a duplicate of the check prerequisite block in main. We could move it to a dedicated function and call it from here too.

Also, as this logic is executed as part of unit tests too, does it make sense to suggest in documentation at L12 to call python hack/test-tekton-kueue-config.py --check-setup before python hack/test-tekton-kueue-config.py?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 411 to 425
# Check annotations
annotations = mutated.get("metadata", {}).get("annotations", {})
for expected_annotation in expected["annotations"]:
self.assertIn(expected_annotation, annotations,
f"Expected annotation {expected_annotation} not found")
self.assertEqual(annotations[expected_annotation], "1",
f"Expected annotation {expected_annotation} to have value '1'")

# Check labels
labels = mutated.get("metadata", {}).get("labels", {})
for expected_label, expected_value in expected["labels"].items():
self.assertIn(expected_label, labels,
f"Expected label {expected_label} not found")
self.assertEqual(labels[expected_label], expected_value,
f"Expected label {expected_label} to have value '{expected_value}', got '{labels.get(expected_label)}'")
Copy link
Member

@filariow filariow Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest checking the whole result of the mutation with assertDictEqual

Suggested change
# Check annotations
annotations = mutated.get("metadata", {}).get("annotations", {})
for expected_annotation in expected["annotations"]:
self.assertIn(expected_annotation, annotations,
f"Expected annotation {expected_annotation} not found")
self.assertEqual(annotations[expected_annotation], "1",
f"Expected annotation {expected_annotation} to have value '1'")
# Check labels
labels = mutated.get("metadata", {}).get("labels", {})
for expected_label, expected_value in expected["labels"].items():
self.assertIn(expected_label, labels,
f"Expected label {expected_label} not found")
self.assertEqual(labels[expected_label], expected_value,
f"Expected label {expected_label} to have value '{expected_value}', got '{labels.get(expected_label)}'")
self.assertDictEqual(mutated, expected)

This will require some changes in the test data too.

I tested with the following changes: git diff. Expand this block, that's collapsed for better readability.
diff --git a/hack/test-tekton-kueue-config.py b/hack/test-tekton-kueue-config.py
index 02d7be8f..0e736fbe 100755
--- a/hack/test-tekton-kueue-config.py
+++ b/hack/test-tekton-kueue-config.py
@@ -116,15 +116,37 @@ TEST_PIPELINERUNS = {
             }
         },
         "expected": {
-            "annotations": [
-                "kueue.konflux-ci.dev/requests-linux-amd64",
-                "kueue.konflux-ci.dev/requests-linux-arm64", 
-                "kueue.konflux-ci.dev/requests-linux-s390x"
-            ],
-            "labels": {
-                "kueue.x-k8s.io/queue-name": "pipelines-queue",
-                "kueue.x-k8s.io/priority-class": "konflux-post-merge-build"
-            }
+            "apiVersion": "tekton.dev/v1",
+            "kind": "PipelineRun",
+            "metadata": {
+                "name": "test-multiplatform-new",
+                "namespace": "default",
+                "creationTimestamp": None,
+                "annotations": {
+                    "kueue.konflux-ci.dev/requests-linux-amd64": "1",
+                    "kueue.konflux-ci.dev/requests-linux-arm64": "1",
+                    "kueue.konflux-ci.dev/requests-linux-s390x": "1",
+                },
+                "labels": {
+                    "pipelinesascode.tekton.dev/event-type": "push",
+                    "kueue.x-k8s.io/queue-name": "pipelines-queue",
+                    "kueue.x-k8s.io/priority-class": "konflux-post-merge-build",
+                }
+            },
+            "spec": {
+                "status": "PipelineRunPending",
+                "taskRunTemplate": {},
+                "pipelineRef": {"name": "build-pipeline"},
+                "params": [
+                    {
+                        "name": "build-platforms",
+                        "value": ["linux/amd64", "linux/arm64", "linux/s390x"]
+                    },
+                    {"name": "other-param", "value": "test"}
+                ],
+                "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
+            },
+            "status": {},
         }
     },
     
@@ -149,7 +171,7 @@ TEST_PIPELINERUNS = {
                             "taskRef": {"name": "build-task"}
                         },
                         {
-                            "name": "build-task-arm64", 
+                            "name": "build-task-arm64",
                             "params": [{"name": "PLATFORM", "value": "linux/arm64"}],
                             "taskRef": {"name": "build-task"}
                         },
@@ -163,14 +185,46 @@ TEST_PIPELINERUNS = {
             }
         },
         "expected": {
-            "annotations": [
-                "kueue.konflux-ci.dev/requests-linux-amd64",
-                "kueue.konflux-ci.dev/requests-linux-arm64"
-            ],
-            "labels": {
-                "kueue.x-k8s.io/queue-name": "pipelines-queue",
-                "kueue.x-k8s.io/priority-class": "konflux-pre-merge-build"
-            }
+            "apiVersion": "tekton.dev/v1",
+            "kind": "PipelineRun",
+            "metadata": {
+                "name": "test-multiplatform-old",
+                "namespace": "default",
+                "creationTimestamp": None,
+                "annotations": {
+                    "kueue.konflux-ci.dev/requests-linux-amd64": "1",
+                    "kueue.konflux-ci.dev/requests-linux-arm64": "1",
+                },
+                "labels": {
+                    "pipelinesascode.tekton.dev/event-type": "pull_request",
+                    "kueue.x-k8s.io/queue-name": "pipelines-queue",
+                    "kueue.x-k8s.io/priority-class": "konflux-pre-merge-build"
+                },
+            },
+            "spec": {
+                "status": "PipelineRunPending",
+                "taskRunTemplate": {},
+                "pipelineSpec": {
+                    "tasks": [
+                        {
+                            "name": "build-task-amd64",
+                            "params": [{"name": "PLATFORM", "value": "linux/amd64"}],
+                            "taskRef": {"name": "build-task"}
+                        },
+                        {
+                            "name": "build-task-arm64", 
+                            "params": [{"name": "PLATFORM", "value": "linux/arm64"}],
+                            "taskRef": {"name": "build-task"}
+                        },
+                        {
+                            "name": "other-task",
+                            "taskRef": {"name": "other-task"}
+                        }
+                    ]
+                },
+                "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
+            },
+            "status": {},
         }
     },
     
@@ -193,11 +247,26 @@ TEST_PIPELINERUNS = {
             }
         },
         "expected": {
-            "annotations": [],
-            "labels": {
-                "kueue.x-k8s.io/queue-name": "pipelines-queue",
-                "kueue.x-k8s.io/priority-class": "konflux-release"
-            }
+            "apiVersion": "tekton.dev/v1",
+            "kind": "PipelineRun",
+            "metadata": {
+                "name": "test-release-managed",
+                "namespace": "default",
+                "creationTimestamp": None,
+                "labels": {
+                    "kueue.x-k8s.io/queue-name": "pipelines-queue",
+                    "kueue.x-k8s.io/priority-class": "konflux-release",
+                    "appstudio.openshift.io/service": "release",
+                    "pipelines.appstudio.openshift.io/type": "managed"
+                }
+            },
+            "spec": {
+                "status": "PipelineRunPending",
+                "taskRunTemplate": {},
+                "pipelineRef": {"name": "release-pipeline"},
+                "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
+            },
+            "status": {},
         }
     },
     
@@ -220,11 +289,26 @@ TEST_PIPELINERUNS = {
             }
         },
         "expected": {
-            "annotations": [],
-            "labels": {
-                "kueue.x-k8s.io/queue-name": "pipelines-queue",
-                "kueue.x-k8s.io/priority-class": "konflux-tenant-release"
-            }
+            "apiVersion": "tekton.dev/v1",
+            "kind": "PipelineRun",
+            "metadata": {
+                "name": "test-release-tenant",
+                "namespace": "default",
+                "creationTimestamp": None,
+                "labels": {
+                    "appstudio.openshift.io/service": "release",
+                    "pipelines.appstudio.openshift.io/type": "tenant",
+                    "kueue.x-k8s.io/queue-name": "pipelines-queue",
+                    "kueue.x-k8s.io/priority-class": "konflux-tenant-release",
+                }
+            },
+            "spec": {
+                "status": "PipelineRunPending",
+                "taskRunTemplate": {},
+                "pipelineRef": {"name": "release-pipeline"},
+                "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
+            },
+            "status": {},
         }
     },
     
@@ -243,11 +327,24 @@ TEST_PIPELINERUNS = {
             }
         },
         "expected": {
-            "annotations": [],
-            "labels": {
-                "kueue.x-k8s.io/queue-name": "pipelines-queue",
-                "kueue.x-k8s.io/priority-class": "konflux-dependency-update"
-            }
+            "apiVersion": "tekton.dev/v1",
+            "kind": "PipelineRun",
+            "metadata": {
+                "name": "test-mintmaker",
+                "namespace": "mintmaker",
+                "creationTimestamp": None,
+                "labels": {
+                    "kueue.x-k8s.io/queue-name": "pipelines-queue",
+                    "kueue.x-k8s.io/priority-class": "konflux-dependency-update",
+                }
+            },
+            "spec": {
+                "status": "PipelineRunPending",
+                "taskRunTemplate": {},
+                "pipelineRef": {"name": "dependency-update-pipeline"},
+                "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
+            },
+            "status": {},
         }
     },
     
@@ -269,11 +366,25 @@ TEST_PIPELINERUNS = {
             }
         },
         "expected": {
-            "annotations": [],
-            "labels": {
-                "kueue.x-k8s.io/queue-name": "pipelines-queue",
-                "kueue.x-k8s.io/priority-class": "konflux-post-merge-test"
-            }
+            "apiVersion": "tekton.dev/v1",
+            "kind": "PipelineRun",
+            "metadata": {
+                "name": "test-integration-test-push",
+                "namespace": "default",
+                "creationTimestamp": None,
+                "labels": {
+                    "kueue.x-k8s.io/queue-name": "pipelines-queue",
+                    "kueue.x-k8s.io/priority-class": "konflux-post-merge-test",
+                    "pac.test.appstudio.openshift.io/event-type": "push",
+                }
+            },
+            "spec": {
+                "status": "PipelineRunPending",
+                "taskRunTemplate": {},
+                "pipelineRef": {"name": "integration-test-pipeline"},
+                "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
+            },
+            "status": {},
         }
     },
     
@@ -295,11 +406,25 @@ TEST_PIPELINERUNS = {
             }
         },
         "expected": {
-            "annotations": [],
-            "labels": {
-                "kueue.x-k8s.io/queue-name": "pipelines-queue",
-                "kueue.x-k8s.io/priority-class": "konflux-pre-merge-test"
-            }
+            "apiVersion": "tekton.dev/v1",
+            "kind": "PipelineRun",
+            "metadata": {
+                "name": "test-integration-test-pr",
+                "namespace": "default",
+                "creationTimestamp": None,
+                "labels": {
+                    "pac.test.appstudio.openshift.io/event-type": "pull_request",
+                    "kueue.x-k8s.io/queue-name": "pipelines-queue",
+                    "kueue.x-k8s.io/priority-class": "konflux-pre-merge-test",
+                }
+            },
+            "spec": {
+                "status": "PipelineRunPending",
+                "taskRunTemplate": {},
+                "pipelineRef": {"name": "integration-test-pipeline"},
+                "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
+            },
+            "status": {},
         }
     },
     
@@ -315,14 +440,27 @@ TEST_PIPELINERUNS = {
             "spec": {
                 "pipelineRef": {"name": "default-pipeline"},
                 "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
-            }
+            },
         },
         "expected": {
-            "annotations": [],
-            "labels": {
-                "kueue.x-k8s.io/queue-name": "pipelines-queue",
-                "kueue.x-k8s.io/priority-class": "konflux-default"
-            }
+            "apiVersion": "tekton.dev/v1",
+            "kind": "PipelineRun",
+            "metadata": {
+                "name": "test-default",
+                "namespace": "default",
+                "creationTimestamp": None,
+                "labels": {
+                    "kueue.x-k8s.io/queue-name": "pipelines-queue",
+                    "kueue.x-k8s.io/priority-class": "konflux-default"
+                },
+            },
+            "spec": {
+                "status": "PipelineRunPending",
+                "taskRunTemplate": {},
+                "pipelineRef": {"name": "default-pipeline"},
+                "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
+            },
+            "status": {},
         }
     }
 }
@@ -408,21 +546,7 @@ class TektonKueueMutationTest(unittest.TestCase):
             mutated = self.run_mutation_test(test_data)
             expected = test_data["expected"]
             
-            # Check annotations
-            annotations = mutated.get("metadata", {}).get("annotations", {})
-            for expected_annotation in expected["annotations"]:
-                self.assertIn(expected_annotation, annotations, 
-                             f"Expected annotation {expected_annotation} not found")
-                self.assertEqual(annotations[expected_annotation], "1",
-                               f"Expected annotation {expected_annotation} to have value '1'")
-            
-            # Check labels
-            labels = mutated.get("metadata", {}).get("labels", {})
-            for expected_label, expected_value in expected["labels"].items():
-                self.assertIn(expected_label, labels,
-                             f"Expected label {expected_label} not found")
-                self.assertEqual(labels[expected_label], expected_value,
-                               f"Expected label {expected_label} to have value '{expected_value}', got '{labels.get(expected_label)}'")
+            self.assertDictEqual(mutated, expected)
     
     def test_all_mutations(self):
         """Test all tekton-kueue mutation scenarios."""

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't use assertDictEqual for the entire PLR since it's not recursice.
I'll use it for asserting the labels and annotations.

@gbenhaim
Copy link
Member Author

/hold

@scoheb
Copy link
Member

scoheb commented Aug 12, 2025

/test appstudio-e2e-tests

2 similar comments
@scoheb
Copy link
Member

scoheb commented Aug 12, 2025

/test appstudio-e2e-tests

@Dannyb48
Copy link

/test appstudio-e2e-tests

- Add hack/test-tekton-kueue-config.py with parameterized unittest
- Test all CEL mutation scenarios: multi-platform, priority assignment, queue config
- Dynamically read image from kustomization and config from development env
- Add GitHub action to run tests on kueue-related PRs

Validates tekton-kueue mutations against expected annotations, labels, and
priority classes using actual container via podman.

Also, update the tekton-kueue config on dev to match staging.

Assisted-By: Cursor
Signed-off-by: Gal Ben Haim <[email protected]>
@gbenhaim gbenhaim force-pushed the test-tekton-kueue-config branch from 5a49b1e to 0351f83 Compare August 14, 2025 09:12
@openshift-ci openshift-ci bot removed the lgtm label Aug 14, 2025
Copy link

openshift-ci bot commented Aug 14, 2025

New changes are detected. LGTM label has been removed.

Copy link
Contributor

Code Review by Gemini

## Code Review

### `components/kueue/development/tekton-kueue/config.yaml`

The change to include `_` replacement in platform names is a good improvement for robustness.

### `hack/test-tekton-kueue-config.py`

**Improvements:**

*   **Add test case for underscore replacement in platform names:**
    The `config.yaml` change now handles `_` characters in platform names (e.g., `windows_amd64`). It would be beneficial to add a specific test case to `TEST_PIPELINERUNS` to ensure this new logic is covered.

    ```diff
    --- a/hack/test-tekton-kueue-config.py
    +++ b/hack/test-tekton-kueue-config.py
    @@ -169,6 +169,34 @@
         }
     },
     
    +    "multiplatform_underscore": {
    +        "name": "Multi-platform pipeline (with underscore in platform name)",
    +        "pipelinerun": {
    +            "apiVersion": "tekton.dev/v1",
    +            "kind": "PipelineRun",
    +            "metadata": {
    +                "name": "test-multiplatform-underscore",
    +                "namespace": "default",
    +                "labels": {
    +                    "pipelinesascode.tekton.dev/event-type": "push"
    +                }
    +            },
    +            "spec": {
    +                "pipelineRef": {"name": "build-pipeline"},
    +                "params": [
    +                    {
    +                        "name": "build-platforms",
    +                        "value": ["windows_amd64", "linux/arm64_v8"]
    +                    }
    +                ],
    +                "workspaces": [{"name": "shared-workspace", "emptyDir": {}}]
    +            }
    +        },
    +        "expected": {
    +            "annotations": {
    +                "kueue.konflux-ci.dev/requests-windows-amd64": "1",
    +                "kueue.konflux-ci.dev/requests-linux-arm64-v8": "1"
    +            },
    +            "labels": {
    +                "kueue.x-k8s.io/queue-name": "pipelines-queue",
    +                "kueue.x-k8s.io/priority-class": "konflux-post-merge-build"
    +            }
    +        }
    +    },
    +
     "release_managed": {
         "name": "Release managed pipeline",
         "pipelinerun": {
    ```

*   **Move `import shutil` to the top of the file:**
    It's a best practice to place all imports at the beginning of the script.

    ```diff
    --- a/hack/test-tekton-kueue-config.py
    +++ b/hack/test-tekton-kueue-config.py
    @@ -19,6 +19,7 @@
     import os
     import yaml
     import unittest
    +import shutil
     from pathlib import Path
     from typing import Dict
     from dataclasses import dataclass
    @@ -209,8 +210,6 @@
             config_path = Path(temp_dir) / "config.yaml"
             pipelinerun_path = Path(temp_dir) / "pipelinerun.yaml"
             
-            # Copy the config file
-            import shutil
             shutil.copy2(self.config_file, config_path)
             
             # Write the PipelineRun
    ```

### `.github/workflows/test-tekton-kueue-config.yaml`

No issues found. The workflow is well-defined and correctly triggers on relevant path changes.

---

**Overall Assessment:**

The new test suite is well-structured, comprehensive, and uses good practices like dynamic configuration loading, temporary directories, and `unittest` subtests. The use of `podman` to run the actual `tekton-kueue` container for mutations provides high confidence in the test results. The suggested improvements are minor and primarily enhance test coverage and code style.

@gbenhaim
Copy link
Member Author

/test appstudio-e2e-tests

@gbenhaim
Copy link
Member Author

/unhold

@gbenhaim
Copy link
Member Author

@psturc do you maybe know what is this issue?

Creating Tekton Bundle:
- Added Pipeline: buildah-remote-pipeline to image
push_pin
Error: error when building/pushing a tekton pipeline bundle: error when pushing a bundle quay.io/redhat-appstudio-qe/test-images:pipeline-bundle-1755171176-apue to a container image registry repo: could not push image to registry as "quay.io/redhat-appstudio-qe/test-images:pipeline-bundle-1755171176-apue": POST https://quay.io/v2/redhat-appstudio-qe/test-images/blobs/uploads/: UNAUTHORIZED: access to the requested resource is not authorized; map[]

@psturc
Copy link
Member

psturc commented Aug 14, 2025

@gbenhaim yes, see this thread, I'm already working on that issue

@gbenhaim
Copy link
Member Author

/retest-required

@gbenhaim gbenhaim merged commit 03b2733 into redhat-appstudio:main Aug 15, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants